[BEAM-6394] Add support to write protobuf data using ProtoParquetReader#34063
[BEAM-6394] Add support to write protobuf data using ProtoParquetReader#34063cyberbeam524 wants to merge 5 commits intoapache:masterfrom
Conversation
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
assign set of reviewers |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @kennknowles for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
@Abacn Could you please take a look? Thanks! |
Abacn
left a comment
Abacn
left a comment
There was a problem hiding this comment.
Thanks! This is a good start. Left a few comments
| abstract Builder toBuilder(); | ||
|
|
||
| // New: flag to indicate using ProtoParquetReader for protobuf data. | ||
| abstract boolean getUseProtoReader(); |
There was a problem hiding this comment.
consider using a enum type parameter (AVRO / PROTO) and defaults to AVRO? This allows flexibility for future additions.
|
|
||
| // Get the configuration and set the property using the literal. | ||
| Configuration conf2 = getConfWithModelClass(); | ||
| conf2.set("parquet.proto.ignore.unknown.fields", "TRUE"); |
There was a problem hiding this comment.
What is the purpose of "ignore.unknown.fields" here? In general we should not set unsafe configure for user if they are not aware of
| } | ||
| } | ||
|
|
||
| return; // exit after using the proto path |
There was a problem hiding this comment.
For readability, I would recommend structure the code, e.g.
getParquetFileReader() {
switch (FORMAT) {
case AVRO: return getParquetAvroReader()
case PROTO: return getParquetProtoReader()
}
}
instead of branching in place
|
Reminder, please take a look at this pr: @kennknowles @johnjcasey |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @robertwb for label java. Available commands:
|
|
waiting on author |
github-actions
bot
added
Next Action: Author
and removed
Next Action: Reviewers
slow-review
labels
- Replaced boolean `useProtoReader` flag with a public `ReaderFormat` enum (AVRO, PROTO) - Default to `ReaderFormat.AVRO` for backward compatibility - Added `withAvroReader()` and `withProtoReader()` builder methods - Refactored`SplitReadFn` to `switch` on `ReaderFormat` and dispatch to Avro or Proto reader - Removed`"parquet.proto.ignore.unknown.fields"` setting from the default path - Update DisplayData to emit `readerFormat.name()` - Update unit tests to assert on `ReaderFormat` and display data accordingly
|
@Abacn Added the following changes according to your comments. Could you please take a look? Thank you!
|
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @kennknowles for label java. Available commands:
|
|
Reminder, please take a look at this pr: @chamikaramj @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @kennknowles for label java. Available commands:
|
|
Reminder, please take a look at this pr: @kennknowles @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @chamikaramj for label java. Available commands:
|
|
Reminder, please take a look at this pr: @chamikaramj @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @robertwb for label java. Available commands:
|
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @ahmedabu98 for label java. Available commands:
|
|
Reminder, please take a look at this pr: @ahmedabu98 @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @kennknowles for label java. Available commands:
|
|
Reminder, please take a look at this pr: @kennknowles @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @ahmedabu98 for label java. Available commands:
|
|
R: @Abacn Keeping this sticky |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
4 participants
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.